Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Perf small gas opti #31

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Perf small gas opti #31

merged 4 commits into from
Jul 5, 2023

Conversation

MerlinEgalite
Copy link
Contributor

@MerlinEgalite MerlinEgalite commented Jul 5, 2023 •
edited
Loading

after / before

Screenshot 2023-07-05 at 11 10 27

@MathisGD
Copy link
Contributor

MathisGD commented Jul 5, 2023

It's more convenient if you provide a gas diff so that we can see what is the improvement

@MerlinEgalite
Copy link
Contributor Author

Yes! Should we add a gas snapshot to the repo @MathisGD @QGarchery ?

@MathisGD
Copy link
Contributor

MathisGD commented Jul 5, 2023

Maybe yes, with the hardhat tests

@MerlinEgalite
Copy link
Contributor Author

Not forge one?

@MathisGD
Copy link
Contributor

MathisGD commented Jul 5, 2023

I think that forge snapshot --diff (which I used to love!) has some issues right now (ref), and forge tests are not a great way to compare gas costs in general because of the fact that everything is made through one unique transaction.

QGarchery
QGarchery requested changes Jul 5, 2023
View reviewed changes
src/Market.sol Outdated Show resolved Hide resolved
@MathisGD
Copy link
Contributor

MathisGD commented Jul 5, 2023

I found this repo yesterday which seems to provide a similar experience to forge snapshot --diff. It would allow contributor to print a nice diff of the gas cost (easier to see).

@MerlinEgalite MerlinEgalite requested a review from QGarchery July 5, 2023 09:31
MathisGD
MathisGD approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@MathisGD MathisGD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice !!

Rubilmax
Rubilmax reviewed Jul 5, 2023
View reviewed changes
Copy link
Collaborator

@Rubilmax Rubilmax left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there are many other large changes to apply before looking for such gas optimizations...
Besides, why are we going back to require checks instead of conditional reverts with custom errors?

This PR is a great illustration though

@MerlinEgalite
Copy link
Contributor Author

MerlinEgalite commented Jul 5, 2023 •
edited
Loading

Besides, why are we going back to require checks instead of conditional reverts with custom errors?

There's a discussion for this specific question #30

@MerlinEgalite MerlinEgalite merged commit 4f4f740 into dev Jul 5, 2023
@MerlinEgalite MerlinEgalite deleted the perf/check-zero branch July 5, 2023 16:23
@MathisGD MathisGD mentioned this pull request Jul 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rubilmax Rubilmax Rubilmax left review comments

@makcandrov makcandrov makcandrov approved these changes

@MathisGD MathisGD MathisGD approved these changes

@QGarchery QGarchery QGarchery approved these changes

@pakim249CAL pakim249CAL Awaiting requested review from pakim249CAL

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MerlinEgalite @MathisGD @Rubilmax @QGarchery @makcandrov